Olive - Task List - Sea Turtles #94
Conversation
| from .routes.task_routes import tasks_bp | ||
| app.register_blueprint(tasks_bp) | ||
| from .routes.goal_routes import goals_bp | ||
| app.register_blueprint(goals_bp) |
There was a problem hiding this comment.
Nice, I like the choice to divide the routes into their own files!
| goal_id = db.Column(db.Integer, primary_key=True) | ||
| id = db.Column(db.Integer, primary_key=True) | ||
| title = db.Column(db.String) | ||
| tasks = db.relationship("Task", backref="goal", lazy=True) |
There was a problem hiding this comment.
If we check out the SQLAlchemy docs for loading items: https://docs.sqlalchemy.org/en/14/orm/loading_relationships.html the docs say:
"The default value of the relationship.lazy argument is "select", which indicates lazy loading."
Doing some more digging about the possible lazy parameter values we might find further info like in https://medium.com/@ns2586/sqlalchemys-relationship-and-lazy-parameter-4a553257d9ef where we see the line:
"lazy = ‘select’ (or True)".
Putting these together we can see that the default value for lazy in a db relationship is select which is the same as True. Since we aren't changing the default value for lazy, we could leave this parameter off and our code would behave the same:
tasks = db.relationship("Task", back_populates="goal")| def to_dict(self): | ||
| return dict( | ||
| id = self.id, | ||
| title = self.title | ||
| ) | ||
|
|
||
| def to_dict_with_tasks(self): | ||
| tasks_info = [task.to_dict() for task in self.tasks] | ||
| return dict( | ||
| id = self.id, | ||
| title = self.title, | ||
| tasks = tasks_info | ||
| ) |
There was a problem hiding this comment.
If we added a new attribute to Goal that needed to be included in every response, we'd have to update 2 functions. This redundancy creates places for potential bugs. If both formats are necessary, I'd think about creating a single function with boolean parameters for the attributes that aren't sent every time. (That could get tedious over time, another option would be to have a single parameter that is a list of all the attributes we want added to the response).
def to_dict(self, include_tasks):
goal_dict = dict(
id = self.id,
title = self.title
)
if include_tasks:
tasks_info = [task.to_dict() for task in self.tasks]
goal_dict["tasks"] = tasks_info
return goal_dict| def to_dict(self): | ||
| if self.goal_id: | ||
| return dict( | ||
| id = self.id, | ||
| goal_id = self.goal_id, | ||
| title = self.title, | ||
| description = self.description, | ||
| is_complete = bool(self.completed_at) | ||
| ) | ||
| else: | ||
| return dict( | ||
| id = self.id, | ||
| title = self.title, | ||
| description = self.description, | ||
| is_complete = bool(self.completed_at) | ||
| ) |
There was a problem hiding this comment.
The feedback on to_dict in Goal applies here too.
| ) | ||
| @classmethod | ||
| def from_dict(cls, data_dict): | ||
| completed_time = data_dict["completed_at"] if "completed_at" in data_dict else None |
There was a problem hiding this comment.
Nice handling for the optional parameter!
| db.session.delete(goal) | ||
| db.session.commit() | ||
|
|
||
| return jsonify({"details": f'Goal {goal.id} "{goal.title}" successfully deleted'}) |
There was a problem hiding this comment.
Nice, I appreciate the informative messages 👍
|
|
||
| tasks_bp = Blueprint("tasks_bp", __name__, url_prefix="/tasks") | ||
|
|
||
| def post_completed_task_to_slack(task): |
There was a problem hiding this comment.
Nice helper function for the Slack API call.
| # **Complete test with assertion about response body*************** | ||
| # ***************************************************************** | ||
| #raise Exception("Complete test with assertion about response body") | ||
| assert response_body == {'details': "No model of type <class 'app.models.task.Task'> with id 1 found"} |
There was a problem hiding this comment.
To ensure we're checking all the relevant data, it might be good to confirm that the database has no records, or that there is no record in the DB with an ID of 1. (This feedback around checking the DB applies to other tests as well.)
| goal = Goal.query.get(1) | ||
| assert goal.title == "Updated Goal Title" |
There was a problem hiding this comment.
Nice checking the DB contents!
| # assertion 2 goes here | ||
| # assertion 3 goes here | ||
| assert response.status_code == 200 | ||
| assert "goal" in response_body |
There was a problem hiding this comment.
Since we check the whole structure of response_body on the line below, we could omit this specific check for goal in response_body.
No description provided.